At the risk of bikeshedding, could we use esUi instead? Otherwise I think the naming rules become a bit confusing because I wouldn't be sure when to use underscores vs. camel case.

I named this es_ui because of the way kibana_react folder was named and assumed that was the convention for folders with shared components. but now I see that a different kibana-react was added recently, with dash instead of underscore 😆

I'll change ours to esUi.

Nit: can we use variables here instead of hardcoding the number of steps?

const lastStep = Object.keys(stepMap).length;

/* ... */

{currentStep < lastStep ? (

/* ... */

{currentStep === lastStep ? (

@sebelga Just wanted to draw your attention to this mechanism since it's similar to the work you've been doing with the form hooks lib. It's used for showing validation after a user has interacted with a field.

Yes, this is yet another way of validating/displaying validation/clearing validation. So here, if I click the name input, then decide to click outside (blur) I have the validation displaying below the field. Not saying this is bad, just saying that this is new to this form and at some point, we should seek to align the validation behavior across apps (thus the form lib work 😊 ).
Like I told you, IMO (my preference) the best UX is to debounce user keystroke events and after a short delay of "silence" (and having inserted a value) display an error if necessary.

Even though I wrote the cron editor originally, I had to re-read the code to understand what on earth the fieldToPreferredValueMap was for. Since we're extracting this common component in this PR, I think we have a good opportunity to make this name more descriptive. What do you think? How about we change it to fieldToDefaultValueMap? Or simply defaultValues?

I must say I don't understand either what "fieldToPreferredValueMap" refers to. If the <CronEditor /> is controlled it should accept a value (object) prop. And if it's not controlled a defaultValue (to set its initial state). Having to provide "cronExpression", "frequency", "fieldToPreferredValueMap" makes it hard to follow.

I assume that it is controlled, so I would have it all grouped under a cronState under the <PolicyStepLogistics />

const [cronState, setCronState] = useState<CronState>({
  expression: DEFAULT_POLICY_SCHEDULE,
  frequency: DEFAULT_POLICY_FREQUENCY,
  fieldToPreferredValueMap: {}, // still don't know what this refers to :))
});

It might just be me but I had to read this a few times to parse the nested ternaries. What do you think of reducing it to a single ternary like this?

  const displayIndices = (indices && typeof indices === 'string')
      ? indices.split(',')
      : indices || undefined;

Why are we checking type? Can it be something other than a string or undefined? I wonder if indices || undefined is necessary.

I also had to spend some time between the isShowingFullIndicesList , displayIndices (which I thought first was a boolean) and ternary in the JSX

(isShowingFullIndicesList
  ? displayIndices
  : [...displayIndices].splice(0, 10)
)

to understand how the logic was working.

What do you think of this other way of writing it?

const INDICES_PREVIEW_COUNT = 10;
const [totalIndexToDisplay, setTotalIndexToDisplay] = useState<number | undefined>(INDICES_PREVIEW_COUNT);
const indicesToArray = typeof indices === 'string'
  ? indices.split(',')
  : indices || [];

const indicesFiltered = indicesToArray.slice(0, totalIndexToDisplay);
const hiddenIndicesCount = indicesToArray.length - INDICES_PREVIEW_COUNT;
const areAllIndicesVisible = indicesFiltered.length > INDICES_PREVIEW_COUNT;

// ... and then in the JSX

<EuiDescriptionListDescription>
  {indicesToArray.length ? (
    <Fragment>
      <EuiText>
        <ul>
          {indicesFiltered.map(index => (
            <li key={index}>
              <EuiTitle size="xs">
                <span>{index}</span>
              </EuiTitle>
            </li>
          ))}
        </ul>
      </EuiText>
      {hiddenIndicesCount > 0 ? (
        <Fragment>
            <EuiSpacer size="s" />
            {areAllIndicesVisible ? (
              <EuiLink onClick={() => setTotalIndexToDisplay(INDICES_PREVIEW_COUNT)}>
                <FormattedMessage
                  id="xpack.snapshotRestore.policyForm.stepReview.summaryTab.indicesCollapseAllLink"
                  defaultMessage="Hide {count, plural, one {# index} other {# indices}}"
                  values={{ count: hiddenIndicesCount }}
                />{' '}
                <EuiIcon type="arrowUp" />
              </EuiLink>
            ) : (
              <EuiLink onClick={() => setTotalIndexToDisplay(undefined)}>
                <FormattedMessage
                  id="xpack.snapshotRestore.policyForm.stepReview.summaryTab.indicesShowAllLink"
                  defaultMessage="Show {count} more {count, plural, one {index} other {indices}}"
                  values={{ count: hiddenIndicesCount }}
                />{' '}
                <EuiIcon type="arrowDown" />
              </EuiLink>
            )}
        </Fragment>
      ): null}
    </Fragment>
  ) : (
    <FormattedMessage
      id="xpack.snapshotRestore.policyForm.stepReview.summaryTab.allIndicesValue"
      defaultMessage="All indices"
    />
  )}
</EuiDescriptionListDescription>

I added this suggestion to #44036 as the same logic is used in 3 other places.

@gchaps Same question here, should this be "Otherwise, the entire snapshot will fail if any primary shards are unavailable"?

What's the UX intention behind this timeout? Do we want to have similar behavior when hitting Run in the table row?

this timeout is to ensure that if the user is in History tab or clicks History tab after running, they get information about the run they just executed. there is no such information in the table, so having timeout from table action isn't necessary. I'll clarify this code comment

there is no such information in the table, so having timeout from table action isn't necessary

I was thinking more if the History tab is visible and the user hits "Run" using the button in the table row.

oh I see. I think this is a good example of a non-redux limitation. in a redux implementation, "Run" would be an action that would trigger data refreshes across multiple components, regardless of where the action executed from. in this implementation, the list component loads data into the table component, but the detail panel loads data independently of either of them. I'm not sure what the best approach is in this case, in order to trigger a refresh of the detail panel from an action taken inside the table. 🤔any ideas?

for this case, the Run from table is hard to access at the same time as the History tab (basically needs an ultrawide window), and there is a refresh button available above the tab as well. however I see this potentially affecting our other apps too, where there are table actions that could affect detail panel data, so I think this merits further discussion.

Seems like a perfect use case of React Context @jen-huang

We normalize indices to an array here and on line 56. Would it be worth extracting this into a common variable?

const indicesArray = Array.isArray(indices) ? indices : indices.split(',');

Why are we prepending index. here?

Because privileges are namespaced by "section". This allows requiring:

• all privileges of a specific section: "index.*"
• a specific privilege "index.some_specific_index_privilege"

It also prevents privilege naming collision. (ES does not seem to be consistent in its naming: "cluster:admin/snapshot" for a cluster privilege, "monitor" for an index privilege)

What is a "section"? Is it just an arbitrary identifier or are we creating formal distinctions between index privileges and cluster privileges?

If it's the latter, then I suggest changing this interface in the future (not necessarily in this PR) to make this clear and so can we avoid special strings which IMO don't communicate their meaning clearly.

<WithPrivileges indexPrivileges={APP_RESTORE_INDEX_PRIVILEGES} clusterPrivileges={EXAMPLE_REQUIRED_CLUSTER_PRIVILEGES}>

A "section" is a "namespace". It allows us to group privileges. Your suggestion would not re-usable as there is an infinite number of groups. This grouping maps to our API contract, when we return the missing privileges (https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/snapshot_restore/server/routes/api/app.ts#L50).

So basically, it simply adding a prefix to the ES privileges to point to the right privileges.

Similar question here.

Nit: Fragment not needed

Can we do this? If we can, then what is the purpose of the Core providing the i18n instance? It seems to me that i18n should be injected.

Nit: When importing from the same folder (or subfolder), I find it clearer to indicate the file name instead of the barrel.

Is this the magic number? 😄

I agree, but using the callBack version. And can we simplify a bit the navigation logic?

const updateCurrentStep = (nextStep: number) => {
  if (!validation.isValid) {
    return;
  }

  setCurrentStep(nextStep);
  setMaxCompletedStep(prev => Math.max(prev, nextStep - 1));
  clearSaveError();
};

const onBack = () => {
  updateCurrentStep(currentStep - 1);
};

const onNext = () => {
  updateCurrentStep(currentStep + 1);
};

I just reviewed the Template UI create wizard and it would be nice to align behaviors:

• Allow clicking on any step (they are all enabled) and do the validation on step-change (preventing the change if the step is invalid)
• When in edit mode all steps should be enabled as the policy is valid (by doing previous point this will be done)

Alison also added a "cancel" button (bottom right-aligned) that I think is useful to exit the flow.

This seems to work because the component is re-rendered, but I think it would be better to update the state with setIndicesOptions()

this is an EUI bug: elastic/eui#2071

Cool. Can you add a comment pointing to it so if we come back later we can check if it has been fixed?

Same comment as above about updating the state.

this is an EUI bug: elastic/eui#2071

Here the user has decided to define index patterns to target the indices. It does not make much sense to offer him a way to enter an index name. What do you think?

Also, there is no validation on the pattern being inserted (the same validation we do in CCR when adding follower patterns). Not a blocker, as it would be much more easy to add it with the Form lib once merged.
@cjcenizal this is already the 3rd place we need to insert a pattern + validation, I think we can make it a priority to have a reusable solution 😊

I think the suggestion list is useful to the user as a way to view matching items as they type, and it's possible they may want to do something with a pattern and single index, such as logstash-*,single-index

It is by default undefined instead of false. And gives the following error when toggled the first time

Same error here, it is undefined by default instead of false,

Nit: it seems like this could be simplified to:

    if (page === 'home') {
      this.changeDocTitle(`${textService.breadcrumbs.home}`);
    } else if (textService.breadcrumbs[page]) {
      this.changeDocTitle(`${textService.breadcrumbs[page]} - ${textService.breadcrumbs.home}`);
    }

Nit: Can we use fromEntries here? And maybe extract a variable for use on ln 91, so we end up doing less work?

const validationErrors = Object.entries(validation.errors).filter(([key, value]) => value.length > 0);
validation.errors = Object.fromEntries(validationErrors);
validation.isValid = validationErrors.length === 0;